Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: tidy use of bpfSupportedMetrics for NewStats() #1629

Merged
merged 1 commit into from
Oct 18, 2024
Merged

chore: tidy use of bpfSupportedMetrics for NewStats() #1629

merged 1 commit into from
Oct 18, 2024

Conversation

maryamtahhan
Copy link
Collaborator

@maryamtahhan maryamtahhan commented Jul 23, 2024 •
edited
Loading

Tidy up the universal passing around of bpfSupportedMetrics for NewStats()

  • Introduce a reasonable default set of bpfSupportedMetrics in the stats package so that these can be used for NewStats() whenever it's invoked.
  • Remove the InitAvailableParamAndMetrics() function and simply initialise the variables in the package at declaration.
  • Remove dependency between bpf and stats packages entirely

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 23, 2024 •
edited
Loading

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: The "chore: tidy universal use of bpfSupportedMetrics" pull request aims to standardize the use of bpfSupportedMetrics across the codebase. It introduces a contained lookup function, replacing the redundant !stats.AvailableBPFMetrics.HardwareCounters.Has(config.CPUTime) checks.

Key Modifications:

  1. Simplified code: The changes reduce code redundancy and improve organization.
  2. Improved maintainability: The consistent use of bpfSupportedMetrics makes the code easier to maintain.
  3. Potential behavioral impact: The changes might affect the initialization of available parameters and metrics.

Observations and Suggestions:

  • The pull request is still a work-in-progress (WIP), indicating that further refinements are needed.
  • It would be beneficial to include unit tests to ensure the new lookup function behaves as expected.
  • A thorough review of the affected code paths is recommended to ensure the changes do not introduce unintended consequences.

Overall, this pull request takes a step towards improving code consistency and maintainability. However, it requires further attention to ensure the changes do not impact the code's behavior.

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 2 times, most recently from 94c4ceb to 4716a13 Compare July 25, 2024 11:14
@maryamtahhan maryamtahhan marked this pull request as ready for review July 25, 2024 11:21
@maryamtahhan
Copy link
Collaborator Author

Hola @dave-tucker @sthaha @rootfs

Would really appreciate a review if you get the chance.

Thanks in advance
Maryam

dave-tucker
dave-tucker requested changes Jul 29, 2024
View reviewed changes
Copy link
Collaborator

@dave-tucker dave-tucker left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up from my earlier comment I feel like we're treating a symptom (bpfSupportedMetrics is being passed everywhere) and not the disease (why does bpfSupportMetrics NEED to be plumbed in so many places).

The bigger problem is that pkg/stats depending on pkg/bpf...
However, pkg/stats is regularly being called from parts of the codebase where a bpfExporter object isn't in scope.

I think the pkg/stats -> pkg/bpf dependency can be removed entirely by simply populating the hashmap with all the possible eBPF features. We're already filtering them before being posted to prometheus - see: CollectResUtilizationMetrics. That should clean up most of the mess.

I don't feel like the cleanup to make pkg/model depend on a maybe-unititialized value in pkg/stats is a good idea - depending on being provided a bpfSupportedStats struct seems fine.

cmd/exporter/exporter.go Outdated Show resolved Hide resolved
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 4716a13 to 6079d67 Compare July 31, 2024 11:13
maryamtahhan
maryamtahhan commented Jul 31, 2024
View reviewed changes
pkg/bpf/exporter.go Outdated Show resolved Hide resolved
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 6079d67 to 862315a Compare July 31, 2024 11:31
@maryamtahhan maryamtahhan changed the title chore: tidy universal use of bpfSupportedMetrics chore: tidy use of bpfSupportedMetrics for NewStats() Jul 31, 2024
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 4 times, most recently from d2e2cc2 to 6d72d65 Compare August 2, 2024 11:40
@maryamtahhan
Copy link
Collaborator Author

maryamtahhan commented Aug 2, 2024 •
edited
Loading

I've moved the stats globals cleanup to #1659 ... and i think this ready for another round of review

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 6d72d65 to 8c2565e Compare August 7, 2024 10:42
@rootfs
Copy link
Contributor

rootfs commented Aug 7, 2024

@maryamtahhan can you rebase to pick the e2e test diagnostics? thanks

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 8c2565e to 1fda657 Compare August 7, 2024 14:49
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 1fda657 to f77516b Compare August 19, 2024 09:09
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 2 times, most recently from 7e46b15 to b11f1f7 Compare September 2, 2024 09:57
@maryamtahhan
Copy link
Collaborator Author

Hi @dave-tucker
I think this is ready for review again. Please let me know if there are any other issues

Thanks in advance

dave-tucker
dave-tucker requested changes Sep 6, 2024
View reviewed changes
pkg/bpf/exporter.go Outdated Show resolved Hide resolved
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 3 times, most recently from 4510ec0 to 718d171 Compare September 9, 2024 09:10
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 718d171 to ce31e90 Compare September 9, 2024 09:18
@maryamtahhan
Copy link
Collaborator Author

I will rebase this again

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 4 times, most recently from 698b1f8 to 1c223d2 Compare September 11, 2024 11:36
@maryamtahhan
Copy link
Collaborator Author

This is rebase now :) \o/ yay!

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 1c223d2 to f95df6d Compare September 30, 2024 13:47
dave-tucker
dave-tucker approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending resolution of merge conflict

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from f95df6d to 7ea948d Compare October 17, 2024 10:11
@maryamtahhan
refactor: bpfSupportedMetrics usage in NewStats()
7ea948d 
Remove the need to pass bpfSupportedMetrics through
n layers of function calls that lead to NewStats()
by adding a reasonable default set of bpfmetrics
that the exporter will/can then override when it's
started.

Signed-off-by: Maryam Tahhan <[email protected]>
@maryamtahhan maryamtahhan merged commit 6de3263 into sustainable-computing-io:main Oct 18, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dave-tucker dave-tucker dave-tucker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maryamtahhan @rootfs @dave-tucker